Skip to content

Replace recursion in QueryRenderer.isSubquery(…) with loop #4025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 15 commits into from
Closed

Replace recursion in QueryRenderer.isSubquery(…) with loop #4025

wants to merge 15 commits into from
+57 −45

Conversation

polyglot-k
Copy link
Contributor

@polyglot-k polyglot-k commented Sep 25, 2025 •
edited
Loading

This pull request refactors the logic for determining whether a given ParserRuleContext represents a subquery in the JpqlQueryRenderer class. The main change is a simplification and clarification of the isSubquery method's traversal logic.

Refactoring and logic simplification:

Refactored the isSubquery method to use an explicit while loop for traversing parent contexts, making the logic easier to follow and avoiding recursive calls.

The method now returns immediately when encountering a SubqueryContext, Update_statementContext, or Delete_statementContext, and only continues traversing if none of these are found.

By replacing recursion with an iterative approach, the method avoids continuously adding frames to the call stack, improving efficiency and stack safety for deeply nested contexts.

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 25, 2025
@mp911de
Copy link
Member

mp911de commented Sep 26, 2025

We have similar code in EqlQueryRenderer and HqlQueryRenderer. Care to address these as well?

@mp911de mp911de added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 26, 2025
@mp911de mp911de self-assigned this Sep 26, 2025
@polyglot-k
Copy link
Contributor Author

@mp911de of course ! i can do that

@polyglot-k
Copy link
Contributor Author

@mp911de
I have fixed all the places where this structure was needed.

I have one question: in the HqlQueryRenderer part, the isSetQuery method is always used as !isSetQuery. Was this maintained for the sake of consistency, or would it be more intuitive to include the negation in the method name itself?

@mp911de
Copy link
Member

mp911de commented Sep 26, 2025

Care to use tabs for indents instead of spaces? Other than that, the pull request looks ready for merge.

@polyglot-k
Copy link
Contributor Author

@mp911de I reflected it!

@polyglot-k
Refactor isSetQuery method in EqlQueryRenderer, HqlQueryRenderer, and…
7e9cb50 
… JpqlQueryRenderer for improved readability

Signed-off-by: KNU-K <[email protected]>
@polyglot-k polyglot-k requested a review from mp911de September 26, 2025 10:16
@polyglot-k
Refactor isSubquery and isSetQuery methods in EqlQueryRenderer and Hq…
8a6094b 
…lQueryRenderer for improved readability

Signed-off-by: KNU-K <[email protected]>
@mp911de
Copy link
Member

mp911de commented Sep 26, 2025

I have one question: in the HqlQueryRenderer part, the isSetQuery method is always used as !isSetQuery. Was this maintained for the sake of consistency, or would it be more intuitive to include the negation in the method name itself?

That's the best name we were able to come up. isNotSetQuery is worse to read than !isSetQuery. We're always happy to get suggestions how to improve.

@polyglot-k
Copy link
Contributor Author

I have one question: in the HqlQueryRenderer part, the isSetQuery method is always used as !isSetQuery. Was this maintained for the sake of consistency, or would it be more intuitive to include the negation in the method name itself?

That's the best name we were able to come up. isNotSetQuery is worse to read than !isSetQuery. We're always happy to get suggestions how to improve.

Yes, I understand that !
Will it merge if I leave it as it is?
@mp911de

@mp911de mp911de changed the title Refactor subquery detection logic in JpqlQueryRenderer Replace recursion in QueryRenderer.isSubquery(…) with loop Sep 29, 2025
@mp911de mp911de closed this in 1334008 Sep 29, 2025
mp911de added a commit that referenced this pull request Sep 29, 2025
@mp911de
Polishing.
9e135f0 
Reformat code and reorder author tags.

See #4025
mp911de pushed a commit that referenced this pull request Sep 29, 2025
@polyglot-k @mp911de
Replace recursion in QueryRenderer.isSubquery(…) with loop.
6a372ed 
Signed-off-by: KNU-K <[email protected]>
Closes #4025
mp911de added a commit that referenced this pull request Sep 29, 2025
@mp911de
Polishing.
05dfa2b 
Reformat code and reorder author tags.

See #4025
@mp911de mp911de added this to the 3.5.5 (2025.0.5) milestone Sep 29, 2025
@mp911de
Copy link
Member

mp911de commented Sep 29, 2025

Thank you for your contribution. That's merged, polished, and backported now.

@polyglot-k
Copy link
Contributor Author

@mp911de oh! thank u

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de Awaiting requested review from mp911de

Assignees

@mp911de mp911de

Labels
type: task A general task
Projects
None yet
Milestone
3.5.5 (2025.0.5)
Development

Successfully merging this pull request may close these issues.
3 participants
@polyglot-k @mp911de @spring-projects-issues